Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proof-of-concept serialization of advanced JSON types, records #1073

Merged
merged 5 commits into from
Sep 5, 2023

Conversation

onionhammer
Copy link
Contributor

Description

Today, complex objects being passed to/from dapr actors must be serializable with DataContractSerializer, which means shotgunning "DataMember" on any primary-constructors (records and C# 8 classes), and difficult workarounds types such as JsonElement.

This PR is a proof-of-concept, as there are areas where this implementation is compromised. A deeper rewrite of Dapr serialization (while still maintaining backwards compatibility) is still necessary.

Issue reference

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@onionhammer onionhammer requested review from a team as code owners April 7, 2023 20:21
Copy link
Contributor

@halspang halspang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is really nice, thanks taking on this work!

Just a few nitpicks and you'll need to fix the DCO check by signing off on your commit (e.g. git commit -s).

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #1073 (5c8e872) into master (17f849b) will decrease coverage by 0.58%.
The diff coverage is 9.34%.

@@            Coverage Diff             @@
##           master    #1073      +/-   ##
==========================================
- Coverage   67.02%   66.45%   -0.58%     
==========================================
  Files         170      171       +1     
  Lines        5693     5750      +57     
  Branches      607      624      +17     
==========================================
+ Hits         3816     3821       +5     
- Misses       1732     1782      +50     
- Partials      145      147       +2     
Flag Coverage Δ
net6 66.45% <9.34%> (-0.58%) ⬇️
net7 66.45% <9.34%> (-0.58%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
src/Dapr.Actors/Client/ActorProxyOptions.cs 70.00% <0.00%> (-7.78%) ⬇️
...torMessageBodyDataContractSerializationProvider.cs 4.16% <0.00%> (ø)
...ors/Communication/ActorMessageBodyJsonConverter.cs 0.00% <0.00%> (ø)
...ation/ActorMessageBodyJsonSerializationProvider.cs 0.00% <0.00%> (ø)
src/Dapr.Actors/DaprHttpInteractor.cs 48.50% <25.00%> (ø)
src/Dapr.Actors/Runtime/ActorRuntimeOptions.cs 74.46% <33.33%> (-2.81%) ⬇️
src/Dapr.Actors/Client/ActorProxyFactory.cs 77.77% <50.00%> (-5.56%) ⬇️
src/Dapr.Actors/Runtime/ActorManager.cs 52.59% <57.14%> (-0.07%) ⬇️
src/Dapr.Actors/Runtime/ActorRuntime.cs 84.21% <100.00%> (+0.21%) ⬆️

Copy link
Contributor Author

@onionhammer onionhammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed nits

Copy link
Contributor

@shivamkm07 shivamkm07 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onionhammer Can you please rebase this PR to master? It seems to be having cherry-picked master commits. Instead of that, rebasing could be more appropriate.

@onionhammer
Copy link
Contributor Author

@shivamkm07 My git fu is weak, so please let me know if what I did there helps

@shivamkm07
Copy link
Contributor

shivamkm07 commented Jul 10, 2023

@shivamkm07 My git fu is weak, so please let me know if what I did there helps

@onionhammer All the extra commits are still there. Seems you have cherry-picked master commits to your branch to update. You must not do that. Instead just merge/rebase master to update the changes. To fix the PR, run the following commands:

   git checkout add-actor-serialization-options
   git branch temp add-actor-serialization-options // create temp branch to have backup
   git reset --hard upstream/master
   git cherry-pick c0edbd0624dd00d05c9e6eb9869993937c3ccfa7
   git cherry-pick 765204feaaad640c4aafa6ee9056ee122e3cd953
   git cherry-pick e2f94b953b63fc18b3c437508c822e83caf60750
   git push -f origin add-actor-serialization-options

Note that above I have used commit hash of the three original commits in the PR. Please ensure these are the ones with the changes you want . Also do re-check everything before doing git push -f as it will override all the changes.
image

@onionhammer
Copy link
Contributor Author

onionhammer commented Jul 10, 2023

Seems you have cherry-picked master commits to your branch to update

This is what Github does, built in, no?
image

@onionhammer onionhammer force-pushed the add-actor-serialization-options branch 2 times, most recently from 892c377 to 8d25615 Compare July 10, 2023 18:56
@onionhammer
Copy link
Contributor Author

@shivamkm07 ok, how about now?

@shivamkm07
Copy link
Contributor

@shivamkm07 ok, how about now?

Good now. Thanks for updating the PR.
@halspang the workflows are awaiting for approval. Can you please approve them?

@shivamkm07
Copy link
Contributor

@onionhammer can you please fix the broken test? One of the integration test is failing

@onionhammer
Copy link
Contributor Author

onionhammer commented Aug 1, 2023

@shivamkm07 To be honest, I'm looking at this now, and I'm simply unable to reproduce the error that the CI is getting, any help investigating this would be much appreciated.

I'm available on discord today if anyone wants to ping me about it.

"error invoke actor method: failed to cast response" is an error I get in production dapr actors occasionally, this branch aside.

Even on master branch, the E2E tests fail intermittently on my machine (except TestWorkflows which fails 100% of the time)

For example:
image

@onionhammer
Copy link
Contributor Author

I would love to get some assistance on this one if anyone has the time; I'm available on discord - IM me

@onionhammer
Copy link
Contributor Author

onionhammer commented Aug 21, 2023

@shivamkm07 still unable to reproduce any issues with the tests locally. Is there a way to run the github actions locally (in a way that works?) Act doesnt seem to work for this project.

@shivamkm07
Copy link
Contributor

@shivamkm07 still unable to reproduce any issues with the tests locally. Is there a way to run the github actions locally (in a way that works?) Act doesnt seem to work for this project.

Tests seem to pass after the changes. Thanks for the fix. Please also fix the DCO.

@onionhammer onionhammer force-pushed the add-actor-serialization-options branch from 9593aaa to cc0c219 Compare August 22, 2023 13:33
Signed-off-by: Erik O'Leary <erik.m.oleary@gmail.com>
Signed-off-by: Erik O'Leary <erik.m.oleary@gmail.com>
…predictable

Signed-off-by: Erik O'Leary <erik.m.oleary@gmail.com>
@onionhammer onionhammer force-pushed the add-actor-serialization-options branch from cc0c219 to e328243 Compare August 22, 2023 13:41
@halspang halspang added this to the v1.12 milestone Sep 5, 2023
@halspang halspang merged commit fd7168f into dapr:master Sep 5, 2023
8 of 10 checks passed
@onionhammer onionhammer deleted the add-actor-serialization-options branch September 5, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants